Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(npm-dependency): upgrades npm to 8.11.0 to resolve security vulnerability. #501

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mdombrowski1997
Copy link

@mdombrowski1997 mdombrowski1997 commented Jul 20, 2022

  • See GHSA-hj9c-8jmm-8c52
  • Our team was made aware of that vulnerability by yarn audit, it does not show up in our dependabot alerts, which may be way it has not received an automatic PR from dependabot in this repo
  • Our team is upgrading to semantic-release v19 to patch some other security alerts we've received from dependabot, and it'd be nice to patch this while we're in ugprading

@mdombrowski1997 mdombrowski1997 marked this pull request as ready for review July 22, 2022 00:11
@travi
Copy link
Member

travi commented Jul 22, 2022

this link does not seem to point to the advisory that you meant for it to point to. could you provide more information? normally we get PRs from renovate and dependabot when a vulnerability it able to be fixed with an upgrade, so i'm wondering why we haven't gotten a PR from either in this case.

@mdombrowski1997
Copy link
Author

Here's the direct link to the GH advisory GHSA-hj9c-8jmm-8c52 (the PR description has been updated as well).

Sorry for the lack of details on this PR yet, I primarily marked it "Ready for review" because its draft status seemed to be preventing checks from running. I've been unable to get tests to pass locally, but I get the same failures on master and this branch, so I was hoping the PR checks would shed some light on my local test failures. The test failures I'm seeing locally are passing on this branch, so it must be something in my local setup, although it does seem this branch broke a test in CI: "✖ integration › Throws error if NPM token is invalid". Locally, no integration tests finish, but 4 set-npmrc-auth tests fail. Pasted below is my output

$ npm run test
npm WARN config global `--global`, `--local` are deprecated. Use `--location=global` instead.

> @semantic-release/npm@0.0.0-development pretest
> npm run lint


> @semantic-release/npm@0.0.0-development lint
> xo


> @semantic-release/npm@0.0.0-development test
> nyc ava -v


  ✔ get-channel › Get default channel
  ✔ get-channel › Get passed channel if valid
  ✔ get-channel › Prefix channel with "release-" if invalid
  ✔ get-pkg › Throw error if missing package.json
  ✔ get-pkg › Throw error if package.json is malformed
  ✔ get-pkg › Throw error if missing package name
  ✔ get-pkg › Verify name and version then return parsed package.json
  ✔ get-pkg › Verify name and version then return parsed package.json from a sub-directory
  ✔ get-registry › Get the registry configured via "NPM_CONFIG_USERCONFIG" for scoped package
  ✔ get-registry › Get default registry
  ✔ get-registry › Get the registry configured in "NPM_CONFIG_REGISTRY"
  ✔ get-registry › Get the registry configured from "publishConfig"    
  ✔ get-registry › Get the registry configured in ".npmrc" and normalize trailing slash
  ✔ get-registry › Get the registry configured in ".npmrc" for scoped package
  ✔ set-npmrc-auth › Set auth with "NPM_TOKEN"
  ✔ get-release-info › Default registry and scoped module
  ✔ get-release-info › Custom registry and scoped module
  ✔ set-npmrc-auth › Set auth with "NPM_USERNAME", "NPM_PASSWORD" and "NPM_EMAIL"
  ✖ set-npmrc-auth › Preserve home ".npmrc" 
  ✖ set-npmrc-auth › Preserve home and local ".npmrc" 
  ✖ set-npmrc-auth › Preserve all ".npmrc" if auth is already configured 
  ✖ set-npmrc-auth › Preserve ".npmrc" if auth is already configured for a scoped package 
  ✔ set-npmrc-auth › Throw error if "NPM_TOKEN" is missing
  ✔ set-npmrc-auth › Emulate npm config resolution if "NPM_CONFIG_USERCONFIG" is set
  ✔ set-npmrc-auth › Throw error if "NPM_USERNAME" is missing
  ✔ set-npmrc-auth › Throw error if "NPM_PASSWORD" is missing
  ✔ verify-config › Verify "npmPublish", "tarballDir" and "pkgRoot" options
  ✔ verify-config › Return SemanticReleaseError if "npmPublish" option is not a Boolean
  ✔ verify-config › Return SemanticReleaseError if "tarballDir" option is not a String 
  ✔ verify-config › Return SemanticReleaseError if "pkgRoot" option is not a String    
  ✔ verify-config › Return SemanticReleaseError Array if multiple config are invalid   
  ✔ set-npmrc-auth › Throw error if "NPM_EMAIL" is missing
  ✔ set-npmrc-auth › Prefer .npmrc over environment variables
  ✔ prepare › Preserve indentation and newline (3.4s)
  ✔ prepare › Updade package.json (3.5s)
  ✔ prepare › Updade package.json and npm-shrinkwrap.json (5.4s)
  ✔ prepare › Updade package.json and package-lock.json (6.3s)
  ✔ prepare › Updade package.json and npm-shrinkwrap.json in a sub-directory (6.3s)
  ✔ prepare › Updade package.json and package-lock.json in a sub-directory (6.5s)
  ✔ prepare › Only move the created tarball if the "tarballDir" directory is not the CWD (7.1s)
  ✔ prepare › Create the package in the "tarballDir" directory (7.1s)
  
  ✖ Timed out while running tests

  27 tests were pending in test\integration.test.js

  ◌ integration › Skip npm auth verification if "npmPublish" is false
  ◌ integration › Skip npm auth verification if "package.private" is true
  ◌ integration › Skip npm token verification if "package.private" is true
  ◌ integration › Throws error if NPM token is invalid
  ◌ integration › Skip Token validation if the registry configured is not the default one
  ◌ integration › Verify npm auth and package
  ◌ integration › Verify npm auth and package from a sub-directory
  ◌ integration › Verify npm auth and package with "npm_config_registry" env var set by yarn
  ◌ integration › Throw SemanticReleaseError Array if config option are not valid in verifyConditions
  ◌ integration › Publish the package
  ◌ integration › Publish the package on a dist-tag
  ◌ integration › Publish the package from a sub-directory
  ◌ integration › Create the package and skip publish ("npmPublish" is false)
  ◌ integration › Create the package and skip publish ("package.private" is true)
  ◌ integration › Create the package and skip publish from a sub-directory ("npmPublish" is false)
  ◌ integration › Create the package and skip publish from a sub-directory ("package.private" is true)
  ◌ integration › Throw SemanticReleaseError Array if config option are not valid in publish
  ◌ integration › Prepare the package
  ◌ integration › Prepare the package from a sub-directory
  ◌ integration › Throw SemanticReleaseError Array if config option are not valid in prepare
  ◌ integration › Publish the package and add to default dist-tag
  ◌ integration › Publish the package and add to lts dist-tag
  ◌ integration › Skip adding the package to a channel ("npmPublish" is false)
  ◌ integration › Skip adding the package to a channel ("package.private" is true)
  ◌ integration › Create the package in addChannel step
  ◌ integration › Throw SemanticReleaseError Array if config option are not valid in addChannel
  ◌ integration › Verify token and set up auth only on the fist call, then prepare on prepare call only

  ─

  set-npmrc-auth › Preserve home ".npmrc"

  Difference:

  + home_config = test␊
    `//custom.registry.com/:_authToken = ${NPM_TOKEN}

  › test/set-npmrc-auth.test.js:62:5



  set-npmrc-auth › Preserve home and local ".npmrc"

  Difference:

  + home_config = test␊
    `cwd_config = test␊
    //custom.registry.com/:_authToken = ${NPM_TOKEN}`

  › test/set-npmrc-auth.test.js:82:5



  set-npmrc-auth › Preserve all ".npmrc" if auth is already configured

  Difference:

  + home_config = test␊
    `//custom.registry.com/:_authToken = ${NPM_TOKEN}

  › test/set-npmrc-auth.test.js:104:5



  set-npmrc-auth › Preserve ".npmrc" if auth is already configured for a scoped package

  Difference:

  + home_config = test␊
    `@scope:registry=http://custom.registry.com␊
    //custom.registry.com/:_authToken = ${NPM_TOKEN}`

  › test/set-npmrc-auth.test.js:125:5

  ─

  4 tests failed
----------------------|---------|----------|---------|---------|-------------------
File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------------------|---------|----------|---------|---------|-------------------
All files             |   43.66 |    43.85 |   64.28 |   42.78 | 
 npm                  |       0 |        0 |       0 |       0 | 
  index.js            |       0 |        0 |       0 |       0 | 1-122
 npm/lib              |   58.57 |    68.49 |   73.33 |   57.66 | 
  add-channel.js      |       0 |        0 |       0 |       0 | 1-45
  get-channel.js      |     100 |      100 |     100 |     100 | 
  get-error.js        |     100 |      100 |     100 |     100 | 
  get-pkg.js          |     100 |      100 |     100 |     100 | 
  get-registry.js     |     100 |      100 |     100 |     100 | 
  get-release-info.js |     100 |      100 |     100 |     100 | 
  prepare.js          |     100 |      100 |     100 |     100 | 
  publish.js          |       0 |        0 |       0 |       0 | 1-43
  set-legacy-token.js |       0 |        0 |       0 |       0 | 1-4
  set-npmrc-auth.js   |     100 |    94.73 |     100 |     100 | 35
  verify-auth.js      |       0 |        0 |       0 |       0 | 1-30
  verify-config.js    |     100 |      100 |     100 |     100 | 
 npm/lib/definitions  |   91.66 |      100 |    87.5 |    90.9 | 
  errors.js           |   91.66 |      100 |    87.5 |    90.9 | 33
----------------------|---------|----------|---------|---------|-------------------

@travi
Copy link
Member

travi commented Jul 22, 2022

it'd be nice to patch this while we're in ugprading

while we're certainly open to raising the minimum to force the tools to install a patched version, the version that this PR updates npm to is within the existing range, so you should be able to resolve the audit alert by updating your lockfile.

even better, we recommend running semantic-release using npx so that it is only installed at release time rather than also at development time. this also prevents your lockfile from growing stale and using older in-range dependency versions.

Locally, no integration tests finish

we have some known problems with our test suite that need attention. are you using an M1 Mac, by chance? if so, you are likely running into the problems described in semantic-release/semantic-release#2478, which also impact this project.

its draft status seemed to be preventing checks from running

since you are a first time contributor, github requires a maintainer to approve execution of the github actions in order to reduce abuse of compute time on their infrastructure. unfortunately, that complicates feedback for legitimate contributors.

we do need to get the verification all passing in order to merge this change, though. i'm happy to keep approving, so feel free to ping me if i dont notice for a while. in the meantime, try uninstalling semantic-release and reinstalling to rebuild that part of your lockfile or migrate to using npx for execution.

@travi
Copy link
Member

travi commented Jul 22, 2022

See GHSA-hj9c-8jmm-8c52

regarding this particular vulnerability, while i understand it may not be impacting your project specifically, this is a good time to at least mention for future readers... we always recommend defining files within the package.json to define an allow list rather than using ignore files. when ignoring files, it is far too easy to forget to ignore a new file that gets added as the package grows.

@travi
Copy link
Member

travi commented Jul 22, 2022

we do need to get the verification all passing in order to merge this change, though

since it looks like the current failure is in the matrix build for node v17, it could be worth removing that non-lts node version from our matrix and replacing it with the upcoming LTS v18

@travi
Copy link
Member

travi commented Jul 22, 2022

i notice that the lockfile only is updated for npm itself. that seems very unlikely if the lockfile was updated with the npm cli since dependencies of npm are almost certainly bumped as part of the npm update. did you updated the lockfile manually, by chance?

@mdombrowski1997
Copy link
Author

are you using an M1 Mac, by chance?

No, Windows 10 on a Dell.

using npx for execution.

Historically, we've been averse to npx over a "normal" dependency for the unpredictability. At the same time that it would benefit us here to get dependency updates for free as soon as they are available, we'd get bugs for free as well. The overhead of manually upgrading packages when necessary has seemed a lower cost than risking an automatic upgrade that breaks something.
With your recommendation of npx, do you see those hesitations as unfounded or overly risk-averse?

did you updated the lockfile manually, by chance?

No, I updated the package.json then ran npm install. I believe that was my entire process for this commit. Aha, I think that's because y'all are already using 8.11.0, so all my commit did was enforce that higher version in the package.json.

@travi
Copy link
Member

travi commented Jul 22, 2022

No, I updated the package.json then ran npm install. I believe that was my entire process for this commit. Aha, I think that's because y'all are already using 8.11.0, so all my commit did was enforce that higher version in the package.json.

thanks for confirming. that does make sense since we were already up to date within the range

@travi
Copy link
Member

travi commented Jul 22, 2022

it appears that the failure that is happening for this PR is also happening for v17 and v18 in the mainline branch, even though the commit originally passed when it was added to the mainline. while i can reproduce the problem, i do not understand why this would be happening.

while that means that the failure was not caused by this change, we still need to fix it before merging. i'm not sure i will have a chance to fix it right away, so help in investigating would be appreciated. it may make sense to fix that problem in a separate PR, but fixing it here would be fine too.

@travi
Copy link
Member

travi commented Jul 22, 2022

No, Windows 10 on a Dell.

knowing that and looking back at your output, i'm wondering if your local failures are related to line endings, especially since the line ending is shown in your output. i wonder if there is a good way to update the tests to be more forgiving of that

@mdombrowski1997
Copy link
Author

As far back as v7.1.3, these tests fail with the same error on my machine. It doesn't seem to be line-ending related, as the test output and some debugging indicates that the failures are legitimate, and the pre-existing home_config lines are completely gone, not just differing by \r. Do you know of any time these tests passed on Windows?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants